Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

fix(filters): prevent crashes when start_date == date.min #10224

Closed
wants to merge 3 commits into from
Closed

fix(filters): prevent crashes when start_date == date.min #10224

wants to merge 3 commits into from

Conversation

p-l-
Copy link
Contributor

@p-l- p-l- commented May 17, 2024 •
edited
Loading

Description

Some reports may include bogus values for start_date, like the date.min value. In that situation, DefectDojo would crash; the bug is not really in DefectDojo, but a bit of defensive programming won't hurt, and we want to avoid a crash even with invalid data.

When that happen, we'll use date.min as start_date rather than date.min - timedelta(days=1) which cannot exist in Python.

Test results

We do have an (internal, home made) scanner that (used to) produce such invalid result, and this patch fixes the issue.

Documentation

N/A

Checklist

This checklist is for your information.

  • Make sure to rebase your PR against the very latest dev bugfix.
  • Bugfixes should be submitted against the bugfix branch.
  • Give a meaningful name to your PR, as it may end up being used in the release notes.
  • Your code is flake8 compliant.
  • Your code is python 3.11 compliant.
  • Add the proper label to categorize your PR.

@dryrunsecurity DryRunSecurity
Copy link

dryrunsecurity bot commented May 17, 2024 •
edited
Loading

Hi there 👋, @DryRunSecurity here, below is a summary of our analysis and findings.

DryRun Security Status Findings
Configured Codepaths Analyzer ❌ 1 finding
AppSec Analyzer ✅ 0 findings
Authn/Authz Analyzer ✅ 0 findings
Sensitive Files Analyzer ✅ 0 findings
Secrets Analyzer ✅ 0 findings

Note

🔴 Risk threshold exceeded. Adding a reviewer if one is configured in .dryrunsecurity.yaml.

notification list: @mtesauro @grendel513

Change Summary (click to expand)

The following is a summary of changes in this pull request made by me, your security buddy 🤖. Note that this summary is auto-generated and not meant to be a definitive list of security issues but rather a helpful summary from a security perspective.

Summary:

The changes in this pull request focus on enhancing the unit tests for the GenericParser class in the test_generic_parser.py file, as well as adding a new test case file generic_bad_date.json to the codebase. These changes are important from an application security perspective as they ensure that the parser can handle edge cases and invalid data in the input reports, which is crucial for providing accurate and reliable results from the security tooling.

The addition of the test_parse_json_bad_date test case specifically checks the behavior of the parser when the date field in the JSON report has an invalid format. This proactive approach to handling such edge cases helps improve the overall reliability and quality of the application security tooling.

Additionally, the changes to the dojo/filters.py file introduce a comprehensive set of Django filters that cover a wide range of functionality, including finding, engagement, product, and user-related filters. These filters include several security-relevant fields and provide flexible and powerful filtering capabilities, which can be useful for managing and prioritizing security vulnerabilities. However, the application security engineer should continue to monitor the usage of these filters and make any necessary adjustments to ensure the security and reliability of the application.

Files Changed:

  1. unittests/tools/test_generic_parser.py: This file has been updated to include a new test case test_parse_json_bad_date that checks the behavior of the GenericParser when the date field in the JSON report has an invalid format.

  2. unittests/scans/generic/generic_bad_date.json: This new file has been added to the codebase and contains a JSON object with a "findings" array that includes a security finding with an invalid "date" field.

  3. dojo/filters.py: This file has been modified to include a comprehensive set of Django filters that cover a wide range of functionality, including finding, engagement, product, and user-related filters. These filters include several security-relevant fields and provide flexible and powerful filtering capabilities.

Powered by DryRun Security

@p-l-
Copy link
Contributor Author

p-l- commented May 17, 2024

I cannot add the bugfix label to my PR, but that's what it is!

@kiblik
Copy link
Contributor

kiblik commented May 18, 2024

Can you also write a unittest for it? If your report contains sensitive info, feel free to anonymize it.

@p-l-
Copy link
Contributor Author

p-l- commented May 18, 2024

@kiblik sure thing. Could you guide me a bit on how to do that (or point to a documentation)? Where should I store the scan result to test?

@kiblik
Copy link
Contributor

kiblik commented May 18, 2024

@kiblik sure thing. Could you guide me a bit on how to do that (or point to a documentation)? Where should I store the scan result to test?

https://documentation.defectdojo.com/contributing/how-to-write-a-parser/

@p-l-
Copy link
Contributor Author

p-l- commented May 19, 2024

OK I did read that but how can I be sure that the code I'm fixing is already tested somehow? Do I need to write a specific test for that? In the meantime I have added a test sample file anyway.

The provided test file would currently create this 500 error when navigating to e.g. /finding/open:
image

@Maffooch
Copy link
Contributor

@p-l- your unit test could first create a finding with the "bad date" and then invoke the filter class that you are making the change to in this PR in the same way that the /finding/open view does. This would ensure that the exception is handled if the test passes

@p-l- p-l- force-pushed the fix-boggus-start_date branch 2 times, most recently from f3f4a31 to f884fb9 Compare May 21, 2024 22:31
@p-l-
Copy link
Contributor Author

p-l- commented May 22, 2024

@Maffooch Thanks. It seems that the results are not sent to the database, so my test won't work. Do you have any idea what the easier way to test it could be?

@Maffooch
Copy link
Contributor

Could put your test here: https://github.com/DefectDojo/django-DefectDojo/blob/master/unittests/test_finding_model.py

@mtesauro
Copy link
Contributor

Closing as stale

@mtesauro mtesauro closed this Jul 31, 2024
@p-l-
Copy link
Contributor Author

p-l- commented Aug 2, 2024

Yep sorry @mtesauro @Maffooch I did not find the time to create a working test :-/

@mtesauro
Copy link
Contributor

mtesauro commented Aug 2, 2024

@p-l-

Yep sorry @mtesauro @Maffooch I did not find the time to create a working test :-/

No need to apologize - I appreciate the work you did to and we can always pick this back up and write a test. Life happens to everyone - feel free to do a PR anytime in the future. 👍

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
unittests
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@p-l- @kiblik @Maffooch @mtesauro